fix: report the rollout's timings when the server ends the run - #8
Merged
Merged
Conversation
`rollout()` logs a final timing summary when its loop finishes. The loop also ends the other way: the server takes the last step it was asked for, closes with plugrl-server-stop, and the agent raises ServerStopped out of an infer. `run()` already treats that as the happy path - the comment there records that it used to escape as an unhandled exception and end a successful run in a traceback. It was not happy enough. The exception left rollout() before the summary, so a run that ended exactly as intended reported no timings at all. What went with them matters more than the timings: env_steps is the only client-side record of how far the rollout got, and it is what a server's global_step has to be reconciled against. Without it there is no way to tell whether the two sides agree about how much work happened. Found while building E9, whose whole point is that reconciliation across several clients. The harness read the count from a log line that a normal finish never printed, so every run looked like it had taken zero steps. The summary now runs in a finally, so it belongs to the rollout however the rollout ends. Tests drive the real loop rather than a stand-in, because the defect was in an exit path and a mocked rollout has none. Both fail against the previous behaviour: one finds no summary at all, the other finds no step count in it. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
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
1 participant
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.
rollout()logs a final timing summary when its loop finishes. The loop also ends the other way: the server takes the last step it was asked for, closes withplugrl-server-stop, and the agent raisesServerStoppedout of an infer.run()already treats that as the happy path — the comment there records that it used to escape as an unhandled exception and end a successful run in a traceback.It was not happy enough. The exception left
rollout()before the summary, so a run that ended exactly as intended reported no timings at all.Why that matters more than timings
env_stepsin that line is the only client-side record of how far the rollout got, and it is what a server'sglobal_stephas to be reconciled against. Without it there is no way to tell whether the two sides agree about how much work happened.How it was found
Building E9, whose entire point is that reconciliation across several clients at once. The harness read the step count from a log line that a normal finish never printed, so every run looked like it had taken zero steps and every row failed its own validity check.
The fix
The summary now runs in a
finally, so it belongs to the rollout however the rollout ends.Tests
tests/test_rollout_reports_on_server_stop.pydrives the real loop rather than a stand-in, because the defect was in an exit path and a mocked rollout has none. Verified both fail against the previous behaviour: one finds no summary at all, the other finds no step count in it.141 passed, 11 skipped.
ruff check --exclude third_party .clean.🤖 Generated with Claude Code